Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SampleCurve/UnevenSampleCurve succeed at reflection #15493

Merged
merged 8 commits into from
Sep 30, 2024

Conversation

mweatherley
Copy link
Contributor

@mweatherley mweatherley commented Sep 28, 2024

(Note: #15434 implements something very similar to this for functional curve adaptors, which is why they aren't present in this PR.)

Objective

Previously, there was basically no chance that the explicitly-interpolating sample curve structs from the Curve API would actually be Reflect. The reason for this is functional programming: the structs contain an explicit interpolation I: Fn(&T, &T, f32) -> T which, under typical circumstances, will never be Reflect, which prevents the derive from realistically succeeding. In fact, they won't be a lot of other things either, notably including bothDebug and TypePath, which are also required for reflection to succeed.

The goal of this PR is to weaken the implementations of reflection traits for these structs so that they can implement Reflect under reasonable circumstances. (Notably, they will still not be FromReflect, which is unavoidable.)

Solution

The function fields are marked as #[reflect(ignore)], and the derive macro for Reflect has FromReflect disabled. (This is not fully optimal, but we don't presently have any kind of "read-only" attribute for these fields.) Additionally, these structs receive custom Debug and TypePath implementations that display the function's (unstable!) type name instead of its value or type path (respectively). In the case of TypePath, this is a bit janky, but the instability of type_name won't generally present an issue for generics, which would have to be registered manually in the type registry anyway, which is impossible because the function type parameters cannot be named.

(And in general, the "blessed" route for such cases would generally involve manually monomorphizing the function parameter away, which also allows access to FromReflect etc. through very ordinary use of the derive macro.)

Testing

Tests in the new bevy_math::curve::sample_curves module guarantee that these are actually Reflect under reasonable circumstances.


Future changes

If and when function item types become Default, these types will need to receive custom FromReflect implementations that exploit it. Such a custom implementation would also be desirable if users start doing things like wrapping function items in Default/FromReflect wrappers that still implement a Fn trait.

Additionally, if function types become nameable in user-space, the stance on Debug/TypePath may bear reexamination, since partial monomorphization through wrappers would make implementing reflect traits for function types potentially more viable.

@mweatherley mweatherley added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 28, 2024
@mweatherley mweatherley added this to the 0.15 milestone Sep 28, 2024
@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Sep 28, 2024
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a reflection perspective this looks good to me!

crates/bevy_math/src/curve/sample_curves.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/curve/sample_curves.rs Show resolved Hide resolved
crates/bevy_math/src/curve/sample_curves.rs Show resolved Hide resolved

/// A curve that is defined by explicit neighbor interpolation over a set of evenly-spaced samples.
#[derive(Clone)]
#[cfg_attr(feature = "serialize", derive(serde::Serialize, serde::Deserialize))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we #[reflect(Serialize, Deserialize)]? Same goes for the other types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into issues with that with the generics, so I'd prefer to dedicate more effort to that in a separate PR. It would be nice though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I always forget how much generic types mess up the type data registrations haha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah; on the other hand, they have a tendency to make them less useful as well, so 🤷

crates/bevy_math/src/curve/sample_curves.rs Show resolved Hide resolved
crates/bevy_math/src/curve/sample_curves.rs Show resolved Hide resolved
crates/bevy_math/src/curve/sample_curves.rs Show resolved Hide resolved
@MrGVSV
Copy link
Member

MrGVSV commented Sep 28, 2024

The reason for this is functional programming: the structs contain an explicit interpolation I: Fn(&T, &T, f32) -> T which, under typical circumstances, will never be Reflect, which prevents the derive from realistically succeeding.

I wonder if we could generate an anonymous-scoped type that wraps the Fn(&T, &T, f32) -> T and then uses remote reflection to power the reflection API for that function pointer. Or maybe even a generic one provided by bevy_reflect. 🤔

@MrGVSV
Copy link
Member

MrGVSV commented Sep 28, 2024

If and when function item types become Default, these types will need to receive custom FromReflect implementations that exploit it.

Not necessarily. Any fields marked #[reflect(ignore)] in a type not marked #[reflect(from_reflect = false)] will have a Default bound by default (you can also give it a custom default constructor if needed).

@mweatherley
Copy link
Contributor Author

Not necessarily. Any fields marked #[reflect(ignore)] in a type not marked #[reflect(from_reflect = false)] will have a Default bound by default (you can also give it a custom default constructor if needed).

That's fair, I was just thinking about #[derive(Reflect)], but we can use #[derive(FromReflect)] separately for that.

I wonder if we could generate an anonymous-scoped type that wraps the Fn(&T, &T, f32) -> T and then uses remote reflection to power the reflection API for that function pointer. Or maybe even a generic one provided by bevy_reflect. 🤔

I was also wondering about stuff of this nature. Maybe worth investigating!

@mweatherley
Copy link
Contributor Author

Looks like the #[reflect(Debug)] stuff didn't work out; I'm content to look into it some other time, though.

@mweatherley mweatherley added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 30, 2024
@alice-i-cecile
Copy link
Member

Merge conflicts are non-trivial; clean those up and I'll press the button.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2024
Merged via the queue into bevyengine:main with commit 93aa2a2 Sep 30, 2024
26 checks passed
@mweatherley mweatherley deleted the sample-curve-reflection branch September 30, 2024 19:35
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
…ine#15493)

(Note: bevyengine#15434 implements something very similar to this for functional
curve adaptors, which is why they aren't present in this PR.)

# Objective

Previously, there was basically no chance that the
explicitly-interpolating sample curve structs from the `Curve` API would
actually be `Reflect`. The reason for this is functional programming:
the structs contain an explicit interpolation `I: Fn(&T, &T, f32) -> T`
which, under typical circumstances, will never be `Reflect`, which
prevents the derive from realistically succeeding. In fact, they won't
be a lot of other things either, notably including both`Debug` and
`TypePath`, which are also required for reflection to succeed.

The goal of this PR is to weaken the implementations of reflection
traits for these structs so that they can implement `Reflect` under
reasonable circumstances. (Notably, they will still not be
`FromReflect`, which is unavoidable.)

## Solution

The function fields are marked as `#[reflect(ignore)]`, and the derive
macro for `Reflect` has `FromReflect` disabled. (This is not fully
optimal, but we don't presently have any kind of "read-only" attribute
for these fields.) Additionally, these structs receive custom `Debug`
and `TypePath` implementations that display the function's (unstable!)
type name instead of its value or type path (respectively). In the case
of `TypePath`, this is a bit janky, but the instability of `type_name`
won't generally present an issue for generics, which would have to be
registered manually in the type registry anyway, which is impossible
because the function type parameters cannot be named.

(And in general, the "blessed" route for such cases would generally
involve manually monomorphizing the function parameter away, which also
allows access to `FromReflect` etc. through very ordinary use of the
derive macro.)

## Testing

Tests in the new `bevy_math::curve::sample_curves` module guarantee that
these are actually `Reflect` under reasonable circumstances.

---

## Future changes

If and when function item types become `Default`, these types will need
to receive custom `FromReflect` implementations that exploit it. Such a
custom implementation would also be desirable if users start doing
things like wrapping function items in `Default`/`FromReflect` wrappers
that still implement a `Fn` trait.

Additionally, if function types become nameable in user-space, the
stance on `Debug`/`TypePath` may bear reexamination, since partial
monomorphization through wrappers would make implementing reflect traits
for function types potentially more viable.

---------

Co-authored-by: Gino Valente <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants